test: integration tests for the tier 1 release fixes - #703

Nil pointer dereference on the post-create lookup failure path (pre-existing, but this function is being touched here and the new diagnosis flow makes create failures more visible): a few lines below at the options.Response handling, when octopus.Releases.GetByID(options.Response.ReleaseID) fails, the error branch still dereferences the nil result:

newlyCreatedRelease, lookupErr := octopus.Releases.GetByID(options.Response.ReleaseID)
if lookupErr != nil {
    cmd.PrintErrf("Warning: cannot fetch release details: %v\n", lookupErr)
    printReleaseVersion(options.Response.ReleaseVersion, newlyCreatedRelease.Assembled, newlyCreatedRelease.ReleaseNotes, nil)

ReleaseService.GetByID returns nil, err on failure, so a transient server error right after a successful create panics the CLI instead of printing the warning.

The finding holds on this branch. pkg/cmd/release/create/create.go:366-369 still reads newlyCreatedRelease.Assembled and .ReleaseNotes on the lookupErr != nil branch, and ReleaseService.GetByID returns nil, err, so a transient failure right after a successful create panics.

Not fixing it here, though: #722 ("fix: don't dereference a nil release when the post-create lookup fails") exists for exactly these lines, and its diff is the fix — it also covers the lookupErr == nil && newlyCreatedRelease == nil case, prints time.Time{}, "" instead of reading off the nil, and has a release creation warns, rather than panicking, when the post-create lookup fails unit test. A second edit to the same four lines on this branch would just be a conflict for whichever lands second.

The dereference is pre-existing on main and reaches this branch unchanged — nothing in the tier-1 merges touched it — so this PR doesn't have to carry it. Happy to be told otherwise if you'd rather it ride along with the integration tests, since the diagnosis flow does make create failures more visible.

The package diagnosis can misattribute an unrelated 5xx and hide the real cause. This branch runs for any 5xx, not just the null-reference case, and MissingPackageVersionsError.Error() does not include the original server message (it is only reachable via Unwrap). If the server 500s for an unrelated reason (timeout, genuine server bug) while the project happens to contain a package with no version in its feed — or the CLI's re-derived baseline disagrees with the server (e.g. a --package override the CLI silently failed to parse but the server accepted) — the user is told to push packages instead of seeing the actual failure.

Consider gating the missing-package diagnosis on strings.Contains(apiError.ErrorMessage, serverNullReferenceMessage) as well, and/or including the wrapped cause text in the error output.

Half of this is already done on this branch, and the other half was tried and deliberately reverted — both on nj/issue-426 (#695), which owns the code, so nothing changed here.

The wrapped cause is now reported. MissingPackageVersionsError.Error() appends the server reported: <cause> unless the cause text is the null-reference message, which says nothing the diagnosis lines don't already say better (pkg/packages/packages.go:241-248, commit c12edf9). So the "told to push packages instead of seeing the actual failure" outcome no longer happens silently: the real 5xx message is in the output alongside the diagnosis.

The gate on serverNullReferenceMessage did land (ea972e2) and was then taken back out, with the reason left in the doc comment on DiagnoseCreateReleaseFailure: the message for this failure varies by server version — current servers answer with "no viable release plans" rather than a null reference — so gating on that string loses the diagnosis on the servers people actually run. What survives of the gate is the fallback: if the diagnosis finds nothing but the message is the null reference, the error is annotated with "the server failed with an unhandled error; this usually means it could not resolve the packages, channel or git reference".

Live check on this branch, which settles the version-variance argument. Ran TestReleaseCreateMissingPackageVersion against a local Octopus instance with the CLI's stderr dumped. Full output:

cannot create release; no version could be found for the following packages:
  - 'package-9baebdb7-...' in step 'step-9baebdb7-...' (feed 'Octopus Server (built-in)')
push the package(s) to the feed, or supply a version with --package or --package-version
the server reported: Octopus API error: There are no viable release plans in any channels using the provided arguments. The following release plans were considered:
Channel: 'Default' (this is the default channel)
  #   Name                Version   Source           Version rules
  1   step-9baebdb7-...   ERROR     Cannot resolve   Allow any version

So on this server the 500 carries "no viable release plans", not "Object reference not set" — a strings.Contains(apiError.ErrorMessage, serverNullReferenceMessage) gate would have suppressed the diagnosis entirely here. And the wrapped cause is visibly in the output, which is the part that addresses "hide the real cause".

So: real risk, now bounded rather than eliminated. If you want it narrower, the shape I'd suggest to #695 is a gate on the union of the known opaque messages (null reference, "no viable release plans") rather than the null reference alone — but that's a decision for that PR.

Regression for names that legitimately contain commas, with no escape hatch. Octopus allows commas in environment/tenant/machine names. Before this change --environment "Dev, East" (one env named Dev, East) worked; now it is unconditionally split into Dev + East and the deploy fails with "cannot find an environment...". The doc comment acknowledges the constraint but there is no way for a user to opt out (quoting doesn't help — the split happens after shell parsing).

The old octo CLI had the same splitting behaviour, so this may be an accepted trade-off — but worth an explicit decision and a mention in the flag help/changelog, since previously-working invocations now break silently.

There is an escape hatch now, and the flag help documents it — both from nj/issue-556 (#692), which owns this code.

ExpandCommaSeparated splits only on unescaped commas (splitOnUnescapedCommas, pkg/executionscommon/executionscommon.go:306-355, commit b77c149), so --environment 'Dev\, East' yields the single value Dev, East. A backslash anywhere else is preserved verbatim, so DOMAIN\host target names are unaffected. Covered by TestExpandCommaSeparated in pkg/executionscommon/executionscommon_test.go.

Every affected flag says so in its help text, e.g.

--environment   Deploy to this environment (can be specified multiple times, or as a
                comma-separated list; escape a comma inside a value as '\,')

and EscapeCommas re-escapes values on the way back out so the printed automation command round-trips.

The other half of your concern is now an error rather than silent: a blank component ("A,,B", a , from an empty variable substitution) fails with --environment has a blank value; check for an empty variable or a stray comma in "A,,B". Use '\,' to include a comma in a value (c64ab93), which also names the escape at the point of failure.

Changelog: d25b40c/b77c149/c64ab93 are fix: commits on #692 and CHANGELOG.md is release-please-generated, so there's nothing to hand-edit — the note comes from those subjects when the release is cut.

Residual, honestly: the split is still the default and the escape is opt-in, so a previously-working --environment "Dev, East" does break on upgrade until the user adds the backslash. That's the octo compatibility trade you describe. Flipping the default is a decision for #692, not this branch.

False "cannot find a tenant" for tenants beyond the first page of a partial-name search. The SDK's Tenants.GetByIdentifier name fallback (GetByName) issues Get(TenantsQuery{PartialName: name}) and scans only the first page of results for an exact match — it never pages. On a space with many tenants whose names share a common substring (e.g. dozens of "Store ..." tenants), a tenant whose exact name lands beyond page 1 returns ErrItemNotFound, and this new resolution step fails a deploy/runbook run that previously worked (the raw name used to be passed straight to the executions API, which matched it fine).

Also note GetByIdentifier uses a direct type assertion on the GetByID error, so a non-APIError failure (network blip) silently falls through to the name lookup rather than being reported.

The pagination half is fixed on this branch — 93ab1c6 on nj/issue-250 (#702), which owns this file. FindTenant deliberately does not use Tenants.GetByIdentifier; the doc comment now says why, in about the same words as your comment. It does GetByID, then falls back to a PartialName query that pages via resultPage.GetNextPage until it finds an exact EqualFold match or runs out of pages (pkg/question/selectors/tenants.go:20-52). Covered by TestFindTenants/finds an exact name match beyond the first page of the partial name search, which serves a first page that doesn't contain the match.

The second half is only partly addressed, and you're right to flag it. The GetByID error branch is:

if errors.As(err, &apiError) && apiError.StatusCode != 404 {
    return nil, err
}

so a non-APIError failure (a network blip) does not return — it falls through to the name lookup, same as before. In practice the same blip usually fails the name query too and that error is returned, so it isn't silent. The residual is the narrow interleaving: blip on the ID call, name query then succeeds and matches nothing, and the user gets cannot find a tenant with the ID or name of 'x' instead of the transport error.

Worth noting the fall-through is also load-bearing: a plain name like Coke produces a 404 from GET /tenants/Coke and must fall through, so the branch can't simply return every error. Tightening it means distinguishing "this looked like an ID and the server said no" from "the call didn't complete" — which is the same conversation as the round-trip comment below (only do the ID lookup when the identifier matches ^Tenants-\d+$), and that would make both problems go away at once. #702's call, not this branch's.

A mixed list of regular + ephemeral environment identifiers can never resolve. selectors.FindEnvironments errors on the first identifier that isn't a regular environment, and the fallback findEphemeralEnvironments errors on the first identifier that isn't ephemeral — so --environment regularEnv,ephemeralEnv now fails client-side with "cannot find an environment ..." even though before this change both names were passed through verbatim for the server to judge. Probably invalid server-side anyway (one channel type per deployment), but if so the current error message points at the wrong thing: it claims the regular env doesn't exist when it does. Resolving each identifier individually (regular-then-ephemeral per item) would handle both this and give a precise error.

Fixed, and in the shape you suggested — 3d8333b on nj/issue-250 (#702), which owns this code. The function the comment is attached to (resolveEnvironmentNames, local to deploy.go) is gone; it is now selectors.ResolveEnvironmentNames (pkg/question/selectors/environments.go:82-124) and it resolves one identifier at a time: regular list first, then the ephemeral list for any identifier the regular list doesn't have. So --environment regularEnv,ephemeralEnv resolves both, and a genuine miss names the identifier that actually went missing instead of blaming the first one in the list.

Two details worth having on the record:

• the ephemeral list is fetched once and lazily, only when some identifier misses the regular list, so the common all-regular case doesn't pay for the extra call. If that endpoint doesn't exist on the server, the error is still reported as "cannot find an environment with the ID or name of 'x'" — the identifier genuinely isn't a regular environment either way.
• deploy and runbook run now call the same function, so the ephemeral fallback isn't duplicated between them any more.

Unit coverage in pkg/question/selectors/find_test.go: TestResolveEnvironmentNames/resolves a mix of regular and ephemeral environments, /doesn't look at ephemeral environments when everything resolves, and /names the environment that is actually missing.

Nothing changed on this branch for it; it arrived with the merge of nj/issue-250.

Silent precedence flip for existing callers. The old executionscommon.FindEnvironments checked the name lookup before the ID lookup; this new implementation checks ID first. Since executionscommon.FindEnvironments is now an alias to this, the change reaches all its existing callers (tenant connect, five target ... create commands, runbook, deploy): in a space where an environment is named the same as another environment's ID, those commands now resolve to a different environment than before. The tests show this is deliberate ("consistent with how projects and tenants resolve") — flagging it because it's an observable behaviour change to commands this PR doesn't otherwise touch, and may deserve a changelog note.

Confirmed, the flip is real and it does reach commands this PR doesn't otherwise touch. origin/main's executionscommon.FindEnvironments checks nameLookup and only falls back to idLookup; the new selectors.FindEnvironments goes through identifierLookup.find, which is ID-then-name. executionscommon.FindEnvironments is now a one-line alias to it, so the new order applies to tenant connect, the five target ... create commands, runbook run's AskQuestions path and deploy's interactive path — not just the automation paths #702 set out to fix.

The blast radius is narrow but not empty: it only differs when one environment is named exactly another environment's ID (Environments-12), which is legal since names are free text. Nothing in the repo depended on the old order — all unit tests pass with ID-first — so this is an unobservable change except in that collision.

While we're enumerating observable changes to those callers, the error text also changed: cannot find environment environments-12 (lowercased identifier, because the old code lowercased before formatting) is now cannot find an environment with the ID or name of 'Environments-12'. That one is strictly better and I'd leave it.

Two things I'd like a decision on, since both are cheap and they point opposite ways:

1. Keep ID-first everywhere for consistency with projects/tenants/channels, and describe the flip in the commit subject on fix: accept IDs as well as names for --channel, --environment and --tenant #702 so release-please renders it (CHANGELOG.md is generated, so a commit note is the only lever)? Or
2. Keep ID-first for the new executions-API resolution and leave executionscommon.FindEnvironments name-first for its existing callers — i.e. don't make it an alias — so tenant connect and target ... create are bit-for-bit unchanged?

I'd lean (1): one rule is easier to explain than two, and the collision that distinguishes them is pathological. But it's your call, and it belongs on #702 rather than here — I've made no change on this branch.

Every automation-mode invocation now pays extra HTTP round trips even when plain names were given. For each tenant supplied by name this is two requests (a guaranteed 404 on GET /tenants/<name>, then the partial-name search), plus GET /environments/all, plus the release lookup — sequentially, on every CI deploy. The same pattern is in runbook run (and there the environment/tenant resolution also runs in interactive mode, where AskQuestions resolves environments again — duplicate /environments/all calls).

A cheap win: only hit the ID lookup when the identifier actually looks like an ID (^Tenants-\d+$ / ^Environments-\d+$), or resolve tenants with a single query instead of per-tenant round trips. Related: selectors.FindEnvironment (singular) previously used a paged partial-name server query and now loads the entire space's environment list to find one environment.

The counts, as the code stands on this branch, so the decision is on real numbers:

• tenants: 2 requests each, and one of them is a guaranteed 404 for plain names. FindTenant always tries GET /tenants/<identifier> before the partialName search. --tenant a,b,c by name is 6 requests, 3 of them 404s. This is the one that scales with input.
• environments: 1 request total, not per identifier. FindEnvironments/ResolveEnvironmentNames call Environments.GetAll() once and match client-side, so --environment a,b,c is one GET /environments/all. Worth correcting: FindEnvironment (singular) did get worse — it used to be a paged partialName query that could stop at the first page, now it loads the whole space's environment list for one lookup. On a space with a lot of environments that's a bigger response, but still one round trip.
• release: 1 request, and on the default output format it is a net saving. origin/main looked the release up after the deploy to print the web link, and did it as FindProject + GetReleaseInProject — two requests. Both are gone (cf2dc4c), so table output is now one request cheaper. It's a true +1 only with --output-format=json|basic, where the link was never printed and so no lookup happened.

Net for a typical CI release deploy --project P --version V --environment dev: +1 (environments/all) on table output, +2 on json/basic. With --tenant by name: 2 more per tenant on top.

The duplicate you predicted in interactive runbook run is real. run.go:263 resolves environments unconditionally (Environments.GetAll), and then run.go:861/875/1084/1098 call executionscommon.FindEnvironments again from the Q&A path, each of which is another GET /environments/all — so an interactive runbook run with --environment given fetches the full environment list at least twice.

The ^Tenants-\d+$ guard you suggest is the right first move and it's cheap: it removes the 404 per named tenant, and as a side effect removes the "network blip falls through to the name lookup" residual on the other thread, because the ID call would only be made when the identifier actually is an ID. The environment duplication wants the other fix — resolve once and thread the result through, which is the altitude point on the thread below.

What I'd like decided: is any of this in scope for the tier-1 fixes, or does it go in the follow-up with the altitude change? My read is that the tenant guard is small enough to land on #702 now (roughly a regexp plus a branch, with a unit test that asserts no GET /tenants/<name> is issued for a plain name), and the environment de-duplication needs the executor-layer refactor and should wait. If you agree I'll raise it on #702 rather than here.

Altitude: the ID-to-name resolution is scattered per command, at differing depths. release create resolves the channel only in the automation branch; release deploy resolves tenants before both modes but environments only in the automation branch; runbook run resolves both unconditionally. The invariant they all enforce ("the executions API only matches by name") belongs to the layer that builds the executions-API commands (pkg/executor/release.go / the runbook executor), where one implementation would cover all three commands, both modes, and any future execution flag — instead of a pattern that has to be remembered (and is already applied inconsistently) in each command. Fine to land as-is for the tier-1 fixes, but worth a follow-up.

Agreed, and the inconsistency is still exactly as you describe on this branch — I checked each one:

• release create: channel resolved only in the automation branch (create.go:314-321).
• release deploy: tenants resolved before the mode split (deploy.go:274-281, so both modes), environments only in the automation branch (deploy.go:388). Interactive mode relies on AskQuestions resolving them separately.
• runbook run: both resolved unconditionally, before the mode split (run.go:262-275).

Three commands, three different placements, and the interactive runbook run path pays for it — it resolves environments at run.go:263 and then AskQuestions calls executionscommon.FindEnvironments again at run.go:861/875 (and 1084/1098 on the by-tag path), so the full environment list is fetched at least twice.

pkg/executor/release.go is the right home: the invariant is a property of the executions-API command shape, not of any one command's flag parsing, and putting it there also fixes the duplication above for free (resolve once, at the point the command is built).

No change here — this PR is the integration tests for the tier-1 fixes and a refactor of the executor's inputs would put the fixes and their reshaping in one changeset. Do you want me to raise the follow-up issue, and if so should it carry the tenant ^Tenants-\d+$ guard from the round-trip thread as well, or keep that separate on #702 where it's a two-line change?

Only stop the deployment for a confirmed missing release.

FindRelease also returns ReleaseNotFoundError{Confirmed: false} for an empty response, including an empty-body 403 or 502 (as its new comment explains). This check ignores Confirmed, so those lookup failures still abort a deployment that previously went straight to the executions API. I reproduced the full command path with an empty-body 403: it exits with could not resolve a release... before submitting the deployment. The existing permission test only covers a 403 with an API-error body, which takes the intended fallback.

Please require releaseNotFound.Confirmed before treating the preflight as fatal; otherwise leave the release ID unset and let the deployment endpoint decide. Add empty-body 403/502 cases alongside the existing permission test, while retaining the structured-404 rejection test.

Actioned in 5890921. The pre-flight now requires releaseNotFound.Confirmed before it returns the error; an unconfirmed ReleaseNotFoundError leaves options.ReleaseID unset and the deployment goes to the executions API exactly as it did before this PR.

Tests, in TestDeployCreate_AutomationMode:

• release deploy proceeds when the release lookup is forbidden with an empty body (403, no body)
• release deploy proceeds when the release lookup hits an empty-bodied gateway error (502, no body)
• release deploy proceeds when the release lookup is forbidden with an error body — your existing permission case, behaviour unchanged

All three assert the same thing (the POST /deployments/create/untenanted/v1 body, no release ID, no web link), so they now share one parametrised body, deploysDespiteFailedReleaseLookup, which takes a function that answers the release request. Otherwise it was a 40-line table entry duplicated three ways.

Both new cases genuinely bite: dropping && releaseNotFound.Confirmed and re-running fails exactly those two and nothing else in the table.

One existing case changed shape. release deploy explains that 'latest' is not a supported release version answered with RespondWithStatus(404, "NotFound", nil) — an empty body, which is now non-fatal, so that case would have started submitting a deployment instead of failing. A real server answers that request with a 404 carrying an APIError body (per the live check on 2026.3.14820 recorded on #696), so the mock now sends that body and the case still asserts a fatal, confirmed rejection; the message just loses the hedge:

cannot find a release with version 'latest' in project 'Fire Project'. 'latest' is not a supported alias, specify an exact version. ...

release deploy reports a release version that doesn't exist (structured 404) is untouched.

Residual, stated plainly: on a server where a bodyless 404 really is the answer for a missing version, --version latest no longer gets the alias hint from the pre-flight — the deployment is submitted and the server's own error surfaces instead. That is the trade this asks for, and the live evidence on #696 says the unconfirmed branch isn't reachable from the server itself, only from something sitting in front of it.

Note on where this lives: these lines reached this branch from nj/issue-294 (#696) via the merge. The finding was raised here so the fix is here; if #696 lands first, the same one-line condition needs the same edit there.

Use a non-default channel to prove the requested channel ID is honoured.

This project has only its default channel, and the assertion expects that same channel. The test therefore also passes if the CLI drops --channel entirely and lets the server select the default. It detects the old ID-as-name error, but does not distinguish correct resolution from silently ignoring the selection.

Create a second, non-default channel in the fixture, pass its ID here, and assert that the resulting release belongs to it. That makes the integration test verify the destination as well as successful release creation.

Actioned in 89cca7d. The project now gets a second channel (createSecondaryChannel, no lifecycle of its own so it inherits the project's, no version rules so any version is valid in it), the case passes that channel's ID, and it asserts the release landed there — plus assert.NotEqual against the default channel, so the "server picked the default" outcome is named as the thing being excluded.

Verified against a live instance (local Octopus on Spaces-1, admin key):

• as written: --- PASS: TestReleaseCreateAndDeployByID/create_accepts_a_channel_ID
• mutation check: patched release create to resolve --channel and then discard it (options.ChannelName = "" after FindChannel), rebuilt the CLI, re-ran — Not equal: expected "Channels-353", actual "Channels-352", i.e. the secondary channel vs the project default.

So the failure mode you describe is now detected, and the previous assertion would have passed through it.

Two deliberate choices:

• the channel is created in the test, not in CreateCommonProject. fixtures.go asks for shared fixture data that doesn't need putting back, and a second channel changes what the server picks for the other tests that call release create with no --channel (TestReleaseCreateBasics, TestReleaseCreateMissingPackageVersion).
• its t.Cleanup is registered before deleteAllReleasesInProject, so cleanups run releases-first and the channel delete isn't refused for having a release in it.

What I could and couldn't run: these tests are gated on OCTOPUS_TEST_URL/OCTOPUS_TEST_APIKEY (no build tag — GetApiClient calls os.Exit(999) when they're unset), so go test ./... can't reach them. I ran them against a local server and all six pass — TestReleaseCreateBasics, TestReleaseListAndDelete, TestReleaseDeployUnknownVersion, TestReleaseCreateMissingPackageVersion, TestReleaseCreateAndDeployByID, TestReleaseDeployCommaSeparatedTargets — but only individually or in small groups. A -run TestRelease run of the whole file starts answering Octopus API error: Rate limit exceeded partway through, which also takes out the fixture teardown, so I can't tell you the file is green end to end in one pass. On my local instance that looks like the box's limiter rather than the tests, but if the nightly job ever fails this way, that's the shape of it.